Optimize VisitorState#getConstantExpression #4586
Conversation
By avoiding a second pass over the string.
There was a problem hiding this comment.
I filed this PR as a result of work in PicnicSupermarket/error-prone-support#1138, where I found out that I overlooked VisitorState#getConstantExpression and instead reinvented the wheel.
| } else { | ||
| sb.append(c); | ||
| } | ||
| if (!(value instanceof CharSequence str)) { |
There was a problem hiding this comment.
I added a second commit that enables a further optimization by accepting any CharSequence; this can be backed out if desired.
| if (c == '\'') { | ||
| sb.append('\''); | ||
| } else { | ||
| sb.append(Convert.quote(c)); |
There was a problem hiding this comment.
Incidentally I recently learned that Convert.quote does the thing we'd want now, but only for JDK 23+: openjdk/jdk@144a08e
There was a problem hiding this comment.
Er, and as a result of that change Convert.quote(char) was replaced with Convert.quote(char, boolean), so this change doesn't work with the latest JDK EA builds.
I'm not sure what the best option is. We could use reflection or MR-JARs to call the method.
There was a problem hiding this comment.
Ah, nice catch! I haven't looked at the MR-JAR setup yet, but indeed reflection should work. TBD whether I'll find some time today for a new pass.
| } else { | ||
| sb.append(c); | ||
| } | ||
| if (!(value instanceof CharSequence str)) { |
There was a problem hiding this comment.
Unfortunately we're not quite ready to start using Java > 11 features in non-test code internally. I can remove the instanceof pattern when importing this, I'm just leaving a note here in case you wonder why it disappears.
By avoiding a second pass over the string.